🟠 [Medium] ECH reconciliation block in TLSX_Parse omits the TLS1.3 version guard used by sibling SNI logic

The new ECH accept/reject reconciliation block runs whenever msgType == client_hello && isRequest && !echProcessingInner && ssl->ctx->echConfigs != NULL && !ssl->options.disableECH. Unlike the parallel logic this PR added in TLSX_SNI_Parse (line ~2517, which explicitly checks IsAtLeastTLSv1_3(ssl->version) with the comment "A TLS 1.2 hello never gets an inner pass..."), this block has no version guard. It is currently benign only because the server's TLSX_ECH is created exclusively under IsAtLeastTLSv1_3 in TLSX_PopulateExtensions (so ech resolves to NULL for a downgraded TLS 1.2 ClientHello and the block no-ops). Relying on that implicit invariant is more fragile than the explicit check used a few thousand lines earlier in the same file. Adding the guard would make the two ECH code paths consistent and self-documenting.

Fix: Add IsAtLeastTLSv1_3(ssl->version) to the guard so the reconciliation block matches the defensive version check the PR added to TLSX_SNI_Parse.

In retrospect this guard never made sense to add anyways. Removed the two instances I previously added.

🔵 [Low] Duplicated client/server invocation block in openssl-ech.sh reject branches

Both openssl_server() and openssl_client() now wrap the test invocation in an if [ "$REJECT" -ne 0 ]; then ... else ... fi where the only differences between the two branches are || true and the expected grep string. The full multi-line $WOLFSSL_CLIENT / $OPENSSL s_client command is copy-pasted in each branch, so a future flag change must be made in two places. This is CI-only scripting (not src/*.c), so it is purely a maintainability nit.

Fix: Optionally de-duplicate the invocation to a single command and branch only on the success assertion. Not blocking.

deduplicated

🟠 [Medium] Server now hard-rejects an accepted ECH handshake when the outer ClientHello SNI did not match a publicName · Behavior Change / Interop

The reconciliation block introduces a new, stricter requirement: when the server successfully decrypts the inner ClientHello on the first flight (state == ECH_WRITE_NONE && innerClientHello != NULL and serverState < SERVER_HELLO_RETRY_REQUEST_COMPLETE), it returns UNKNOWN_SNI_HOST_NAME_E unless ech->extensions carries a TLSX_SERVER_NAME (i.e. the outer SNI matched a configured echConfig->publicName). The old EchStateSNI machinery did not gate ECH acceptance on the outer SNI matching the public name. A peer that omits the outer SNI entirely, or sends an outer SNI that does not match any publicName, will now be rejected even though the inner decrypted successfully and ECH could be accepted. wolfSSL's own client always sends the public name (set in TLSX_ECH_Use), so wolfSSL<->wolfSSL and the tests pass, but this is stricter than the ECH spec requires (the public_name is a SHOULD on the client) and may affect interop with third-party clients.

Fix: Confirm this hard-fail-on-missing-public-name is the intended policy. If interop with clients that send a different/absent outer SNI must be preserved, consider downgrading to a permissive path (accept ECH and route on the inner SNI) rather than returning UNKNOWN_SNI_HOST_NAME_E.

This is stricter but probably for the better. A client that sends a different public name would stick-out (bad! I don't want to encourage that). The option of omitting the SNI is an interesting idea but is another potential method of fingerprinting..

If someone needs it to be more lenient in the future then an API could be added at that time.

🟠 [Medium] New UNKNOWN_SNI_HOST_NAME_E error path is not exercised by tests · Missing Tests

The PR adds a new server-side rejection path that returns UNKNOWN_SNI_HOST_NAME_E when the outer ClientHello does not carry a public-name SNI on the first flight, yet none of the added/updated tests drive a ClientHello whose outer SNI is missing or does not match a publicName while the inner still decrypts. All new ECH tests (test_wolfSSL_Tls13_ECH_wire_sni, the bad_configs_ex scenarios) rely on the wolfSSL client which always emits the public-name SNI, so this branch is never taken. Error paths introduced by a PR should be covered.

Fix: Add a test that produces an outer ClientHello with no public-name SNI (or a non-matching outer SNI) while the inner decrypts, and assert the handshake fails with UNKNOWN_SNI_HOST_NAME_E. This both documents and locks in the intended behavior of finding #1.

UNKNOWN_SNI_HOST_NAME was actually the wrong thing to report in this case, changed it to INVALID_PARAMETER so it sends illegal_parameter (as the RFC desires: section 7.1).

Added a test: test_wolfSSL_Tls13_ECH_outer_sni_mismatch

🔵 [Low] Duplicated install/restore swap boilerplate in TLSX_GetSizeWithEch and TLSX_WriteWithEch · Code Duplication / Maintainability

Both functions now repeat the same pattern: find ECH, if (TLSX_EchShouldHideInner(ech)) { prepended = TLSX_EchSwapExtensions(...); installed = 1; } before the work, then if (installed) { prepended = TLSX_EchSwapExtensions(..., prepended); if (ret == 0 && prepended != 0) ret = BAD_STATE_E; } after. This install/restore-with-BAD_STATE_E-check boilerplate is identical in both call sites and is easy to get subtly out of sync when one is edited.

Fix: Consider small helpers (e.g. TLSX_EchInstallPublic/TLSX_EchRestorePublic) to share the install/restore-and-verify logic between the size and write paths. Optional cleanup; behavior is correct as written.

added helpers TLSX_EchConcealExtensions, TLSX_EchExposeExtensions

🟠 [Medium] ECH outer ClientHello reconciliation may falsely reject valid handshakes (new public-name SNI gate) · Logic

Both the review and review-security modes flagged the new outer-CH reconciliation gate at the end of TLSX_Parse, which returns INVALID_PARAMETER when ECH is accepted (ech->state == ECH_WRITE_NONE && ech->innerClientHello != NULL, serverState < SERVER_HELLO_RETRY_REQUEST_COMPLETE) but no TLSX_SERVER_NAME was recorded on ech->extensions. The review mode identifies a concrete bug (Medium/SUGGEST): when the server itself configures wolfSSL_UseSNI() with a host name equal to the ECH public name, TLSX_SNI_Parse sets matched=1 first and skips the publicName block (workingConfig stays NULL), so the SNI is routed to ssl->extensions instead of ech->extensions; the reconciliation then finds no SNI on ech->extensions and aborts an otherwise valid ECH-accepted handshake. This combination is plausible because the public name is typically a real fronting host with a matching cert. The review-security mode rates the same gate Info (an intentional, tested tightening per RFC 9849 via test_wolfSSL_Tls13_ECH_outer_sni_mismatch) and notes there is no security/DoS impact, since only the offending client's own handshake fails and it is not attacker-amplifiable; it flags that spec-compliant peers omitting the outer SNI or using an empty public_name will now be rejected. The two severity views differ (Medium bug vs Info awareness); the stricter Medium is retained.

Fix: Relax the reconciliation check to also accept a public-name SNI recorded on ssl->extensions, or make TLSX_SNI_Parse always route a publicName-matching SNI to ech->extensions even when a server-configured SNI already matched (run the publicName check even when matched is true). Confirm the stricter outer-SNI requirement is the intended policy for all supported ECH peers (including those that omit the outer SNI or use an empty public_name); if broader interop leniency is later desired, skip the hard abort when no server-side SNI policy is configured. Add a regression test for a server that configures wolfSSL_UseSNI() with a host name equal to the ECH public name and accepts ECH.

🔵 [Low] Redundant TLSX_Find(TLSX_ECH) lookups in TLSX_SNI_Parse · Style

The ECH extension is looked up twice on the same ssl->extensions list within one call: once to set checkPublic (TLSX_Find(ssl->extensions, TLSX_ECH) != NULL) and again later to populate echX/ech for the publicName comparison. The earlier lookup discards the result. Caching the echX/ech pointer once would avoid the duplicate list walk and make the two ECH-dependent branches share state.

Fix: Resolve echX/ech once near the top of the server block and reuse it for both the checkPublic decision and the publicName comparison. Optional cleanup; not a correctness issue.

⚪ [Info] ForceZero used to zero-initialize a non-secret Hpke struct · Convention

ForceZero(ech->hpke, sizeof(Hpke)) is added to zero the freshly-XMALLOC'd Hpke struct before wc_HpkeInit. ForceZero is intended for wiping secrets so the compiler cannot elide it; using it for plain initialization is unusual (XMEMSET is the conventional init). This is harmless and consistent with the surrounding ForceZero(ech, sizeof(WOLFSSL_ECH)), so it is cosmetic. The unconditional wc_HpkeFreeKey() call (NULL ephemeralKey) introduced alongside it is safe because wc_ecc_key_free/wc_curve25519_free both null-check.

Fix: Use XMEMSET(ech->hpke, 0, sizeof(Hpke)) for initialization, reserving ForceZero for secret wiping. Take it or leave it; no functional impact.